Skip to content

Conversation

Kariiem
Copy link
Contributor

@Kariiem Kariiem commented Jun 11, 2024

This PR removes all code related to HaskellTarget, makes ArrayTarget on by default independent of the command line arguments and removes extra arguments to several functions that needed to check for the target.

@int-index @Ericson2314 @sgraf812 I hope that a single commit PR is acceptable. I will do the same for --g/ghc flag and HAPPY_GHC.

Copy link
Collaborator

@sgraf812 sgraf812 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me.

Perhaps we should warn the user when they do not pass -a that the code-based recursive ascent parser has now been removed, hinting that they can get rid of the warning by passing -a (which is a bit strange). In a few versions, we can then remove this warning again. Opinions, @int-index, @andreasabel?


Also be sure to mention #268 in the commit message.

@int-index
Copy link
Collaborator

I'm surprised to see no changes to the test suite.

@sgraf812
Copy link
Collaborator

sgraf812 commented Jun 11, 2024

Good point; I think we can now get rid of all targets in tests/Makefile that use -a. Roughly

diff --git a/tests/Makefile b/tests/Makefile
index 84938bd..3e97c35 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -56,42 +56,24 @@ TEST_HAPPY_OPTS = --strict
 %.n.hs : %.ly
 	$(HAPPY) $(TEST_HAPPY_OPTS) $< -o $@
 
-%.a.hs : %.ly
-	$(HAPPY) $(TEST_HAPPY_OPTS) -a $< -o $@
-
 %.g.hs : %.ly
 	$(HAPPY) $(TEST_HAPPY_OPTS) -g $< -o $@
 
 %.gc.hs : %.ly
 	$(HAPPY) $(TEST_HAPPY_OPTS) -gc $< -o $@
 
-%.ag.hs : %.ly
-	$(HAPPY) $(TEST_HAPPY_OPTS) -ag $< -o $@
-
-%.agc.hs : %.ly
-	$(HAPPY) $(TEST_HAPPY_OPTS) -agc $< -o $@
-
 %.n.hs : %.y
 	$(HAPPY) $(TEST_HAPPY_OPTS) $< -o $@
 
-%.a.hs : %.y
-	$(HAPPY) $(TEST_HAPPY_OPTS) -a $< -o $@
-
 %.g.hs : %.y
 	$(HAPPY) $(TEST_HAPPY_OPTS) -g $< -o $@
 
 %.gc.hs : %.y
 	$(HAPPY) $(TEST_HAPPY_OPTS) -gc $< -o $@
 
-%.ag.hs : %.y
-	$(HAPPY) $(TEST_HAPPY_OPTS) -ag $< -o $@
-
-%.agc.hs : %.y
-	$(HAPPY) $(TEST_HAPPY_OPTS) -agc $< -o $@
-
-CLEAN_FILES += *.n.hs *.a.hs *.g.hs *.gc.hs *.ag.hs *.agc.hs *.info *.hi *.bin *.exe *.o *.run.stdout *.run.stderr
+CLEAN_FILES += *.n.hs *.g.hs *.gc.hs *.info *.hi *.bin *.exe *.o *.run.stdout *.run.stderr
 
-ALL_TEST_HS = $(shell echo $(TESTS) | sed -e 's/\([^\. ]*\)\.\(l\)\{0,1\}y/\1.n.hs \1.a.hs \1.g.hs \1.gc.hs \1.ag.hs \1.agc.hs/g')
+ALL_TEST_HS = $(shell echo $(TESTS) | sed -e 's/\([^\. ]*\)\.\(l\)\{0,1\}y/\1.n.hs \1.g.hs \1.gc.hs/g')
 
 ALL_TESTS = $(patsubst %.hs, %.run, $(ALL_TEST_HS))
 
@@ -118,15 +100,15 @@ check.%.y : %.y
 all :: $(CHECK_ERROR_TESTS) $(ALL_TESTS)
 
 check-todo::
-	$(HAPPY) $(TEST_HAPPY_OPTS) -ad Test.ly
+	$(HAPPY) $(TEST_HAPPY_OPTS) -d Test.ly
 	$(HC) Test.hs -o happy_test
 	./happy_test
 	-rm -f ./happy_test
-	$(HAPPY) $(TEST_HAPPY_OPTS) -agd Test.ly
+	$(HAPPY) $(TEST_HAPPY_OPTS) -gd Test.ly
 	$(HC) Test.hs -o happy_test
 	./happy_test
 	-rm -f ./happy_test
-	$(HAPPY) $(TEST_HAPPY_OPTS) -agcd Test.ly
+	$(HAPPY) $(TEST_HAPPY_OPTS) -gcd Test.ly
 	$(HC) Test.hs -o happy_test
 	./happy_test
 	-rm -f ./happy_test

(Some of those targets appear to be duplicated. Weird.)

@Kariiem Kariiem force-pushed the array-target-only branch from 8669956 to 6cbf2b4 Compare June 11, 2024 22:54
@Kariiem Kariiem force-pushed the array-target-only branch from 3b391d5 to b9257a3 Compare June 12, 2024 09:01
@Kariiem
Copy link
Contributor Author

Kariiem commented Jun 12, 2024

I commited the changes to the Makefile.

@sgraf812
Copy link
Collaborator

sgraf812 commented Jun 20, 2024

@Ericson2314 @int-index it would be great to make progress here, so that the PR stack that @Kariiem has to wrangle does not become too large. The goal is to merge the changes in #272 bit by bit, starting with the least controversial ones.

To me, this PR looks good. It's probably good to run the CI pipeline just in case (which needs maintainer approval.)

@int-index
Copy link
Collaborator

It's probably good to run the CI pipeline just in case (which needs maintainer approval.)

I approved the CI run (again). Is there any way to allow CI to run in all @Kariiem's PRs?

@sgraf812 I actually thought you had maintainer access, maybe you could request it? (Admins of the haskell group should be able to grant those permissions).

Comment on lines 302 to 306
#if defined(HAPPY_GHC)
happyTcHack :: Happy_GHC_Exts.Int# -> a -> a
happyTcHack x y = y
{-# INLINE happyTcHack #-}
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these lines should not have been removed; that would explain the CI errors.

Undoing an accidental removal
@sgraf812
Copy link
Collaborator

@sgraf812 I actually thought you had maintainer access, maybe you could request it? (Admins of the haskell group should be able to grant those permissions).

Alright, I requested and got maintainer access on Matrix!

@sgraf812 sgraf812 merged commit d9b9980 into haskell:master Jun 20, 2024
@sgraf812
Copy link
Collaborator

I merged; the remaining CI errors appear to be unrelated and are hopefully fixed by #279.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants